Improved invoice PDF generation flow#16251
Improved invoice PDF generation flow#16251rogyar wants to merge 3 commits intomagento:2.3-developfrom
Conversation
magento-engcom-team
added
Partner: Atwix
|
Hi @rogyar. Thank you for your contribution
For more details, please, review the Magento Contributor Assistant documentation |
| )->date('Y-m-d_H-i-s'); | ||
| return $this->_fileFactory->create( | ||
| 'invoice' . $date . '.pdf', | ||
| "invoice_$invoiceId.pdf", |
There was a problem hiding this comment.
Would it better to filter $invoiceId before? I see we are taking it directly from a request.
I am concerned about a potential path traversal attack.
I suppose changing line 51 would be just fine: $invoiceId = (int) $this->getRequest()->getParam('invoice_id');.
There was a problem hiding this comment.
Thanks, Riccardo. That's a very good point. I've added the mentioned change
|
I am wandering if it does worth to move the focus to
|
|
@rogyar , I am putting this onhold while we are deciding which is the best way. |
|
Hi @phoenix128. I'm closing this PR since we have a better solution. I've created another PR for fixing the same issue #16401 . Thanks a lot for the brainstorm! ;) |
3 participants
Description
Currently, in an attempt to print an invoice from admin panel, the corresponding PDF file is generated directly in
vardirectory of the Magento installation. Very often this approach leads to a mess in thevar directory if the invoice printing action takes place frequently during a day.This PR brings a concept for two improvements:
var/pdfdirectory so thevardirectory root is clean.Fixed Issues (if relevant)
Manual testing scenarios
Open an existing invoice in the admin panel.
Click the "Print" button.
You should have the invoice downloaded.
You should have a copy of the invoice generated in the
var/pdfdirectory instead ofvar.Click on the "Print" button once again
The
var/pdfdirectory should not contain an additional copy of the same invoiceThe same issue is fare for shipments and credit memos. I'm going to address those parts in a separate PR or within a scope of this one. I just want to make sure that the proposed concept is fine and we are good to move forward.
Thank you